-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: add configure option --v8-enable-short-builtin-calls #42109
build: add configure option --v8-enable-short-builtin-calls #42109
Conversation
Review requested:
|
Is this a breaking change for native addons? |
No, this is not a breaking change. |
For the failed check "First commit message adheres to guidelines at https://goo.gl/p2fr5Q /...", I have modified the title format of this commit, but don't know how to rerun this failed check. |
Should we be able to measure the performance improvement with our micro benchmarks? |
We have tested the performance with some internal node.js application benchmarks, and short-builtin-calls(together with sparkplug) can bring +6% performance increase. |
@nodejs/build @nodejs/v8 |
Benchmark run on the Results
|
I have modified the commit message to fix the lint error and now all checks have passed. |
For CI tests, this commit passed all of them about 1 weeks ago. |
This comment was marked as outdated.
This comment was marked as outdated.
Is it possible that the consistent failures on fedora-latest-x64 in CI are due to the change in this PR? I'm not seeing it anywhere else, and it's consistent here.... |
Thx for the info. I was using retry button. Looks I click the wrong button.
Old build works fine, not sure why. https://ci.nodejs.org/job/node-test-pull-request/43380/ Looks like something wrong with Jenkins cc @richardlau @nodejs/build |
I rebooted test-digitalocean-fedora34-x64-1 and the CI now passes on it 🤷. |
Landed in aa52873 |
@qdaoming-intel Thx for the contribution. (sorry the commit-queue mess and take so long to merge) |
@gengjiawen Thank you and all other people's support. |
Add configure option --v8-enable-short-builtin-calls and enable it by default on x86_64 platform. PR-URL: nodejs#42109 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
This commit breaks build/compiling C++ in v16.x, is says something like: |
Please don't merge this commit to v16.x, because this commit depends on some new feature in V8, and v16.x doesn't have such feature. |
Add configure option --v8-enable-short-builtin-calls
and enable it by default on x86_64 platform.